Skip to content
This repository has been archived by the owner on May 16, 2023. It is now read-only.

[elasticsearch] cleanup remaining pv(c) after goss tests #336

Closed
wants to merge 3 commits into from
Closed

[elasticsearch] cleanup remaining pv(c) after goss tests #336

wants to merge 3 commits into from

Conversation

jmlrt
Copy link
Member

@jmlrt jmlrt commented Oct 17, 2019

  • Chart version not bumped (the versions are all bumped and released at the same time)
  • README.md updated with any new values or changes
  • Updated template tests in ${CHART}/tests/*.py
  • Updated integration tests in ${CHART}/examples/*/test/goss.yaml

@jmlrt jmlrt added the enhancement New feature or request label Oct 17, 2019
@jmlrt jmlrt requested a review from Crazybus October 17, 2019 17:32
@jmlrt jmlrt self-assigned this Oct 17, 2019
Crazybus
Crazybus reviewed Oct 18, 2019
View reviewed changes
elasticsearch/examples/6.x/Makefile Outdated
@@ -13,3 +13,4 @@ test: install goss

purge:
helm del --purge $(RELEASE)
for pvc in $$(kubectl get pvc -o name | grep $(RELEASE)); do kubectl delete $$pvc; done
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grep is potentially dangerous since it will match partial release names. If someone has a release called elasticsearch it will delete all PVCs with elasticsearch in the name. Is it possible to do this based on labels like this?:

helm-charts/helpers/examples.mk

Lines 3 to 7 in 340868c

GOSS_SELECTOR ?= release=$(RELEASE)
STACK_VERSION := 7.4.0
goss:
GOSS_CONTAINER=$$(kubectl get --no-headers=true pods -l $(GOSS_SELECTOR) -o custom-columns=:metadata.name | sed -n 1p ) && \

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 1e5616b

elasticsearch/examples/6.x/Makefile Outdated
@@ -13,3 +13,4 @@ test: install goss

purge:
helm del --purge $(RELEASE)
for pvc in $$(kubectl get pvc -o name | grep $(RELEASE)); do kubectl delete $$pvc; done
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the pvc part is missing here.

Suggested change
for pvc in $$(kubectl get pvc -o name | grep $(RELEASE)); do kubectl delete $$pvc; done
for pvc in $$(kubectl get pvc -o name | grep $(RELEASE)); do kubectl delete pvc $$pvc; done

You could also drop the for loop and just do kubectl delete pvc $$(kubectl get pvc -o name | grep $(RELEASE))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the pvc part is missing here.

Actually with the -o name, k8s resources are already prefixed by their kind:

$ kubectl get pvc -o name | grep helm-logstash-oss
persistentvolumeclaim/helm-logstash-oss-logstash-helm-logstash-oss-logstash-0

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also drop the for loop and just do kubectl delete pvc $$(kubectl get pvc -o name | grep $(RELEASE))

Nice I didn't know kubectl delete could accept more than one resource in argument, I prefer avoiding the for loop if needed 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 1e5616b

@jmlrt jmlrt requested a review from Crazybus October 18, 2019 09:06
Crazybus
Crazybus previously approved these changes Oct 18, 2019
View reviewed changes
Copy link
Contributor

@Crazybus Crazybus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jmlrt
[elasticsearch] cleanup remaining pv(c) after goss tests
00cce78
@jmlrt
[elasticsearch] improve pvc purge
0a9304e 
`release` label need to be added StatefulSet `spec.selector.matchLabels` to be included in pvc so we can select only pvc matching release
@jmlrt
Copy link
Member Author

jmlrt commented Oct 18, 2019

jenkins test this please

@jmlrt jmlrt requested a review from Crazybus October 18, 2019 17:27
Crazybus
Crazybus previously approved these changes Oct 21, 2019
View reviewed changes
Copy link
Contributor

@Crazybus Crazybus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jmlrt
Copy link
Member Author

jmlrt commented Oct 21, 2019

jenkins test this please

1 similar comment
@jmlrt
Copy link
Member Author

jmlrt commented Oct 21, 2019

jenkins test this please

jmlrt
jmlrt commented Oct 21, 2019
View reviewed changes
elasticsearch/templates/statefulset.yaml
@@ -18,6 +18,7 @@ spec:
selector:
matchLabels:
app: "{{ template "uname" . }}"
release: {{ .Release.Name | quote }}
Copy link
Member Author

@jmlrt jmlrt Oct 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like changing spec.selector.matchLabels during statefulset upgrade is failing:

Error: UPGRADE FAILED: StatefulSet.apps "upgrade-master" is invalid: spec: Forbidden: updates to statefulset spec for fields other than 'replicas', 'template', and 'updateStrategy' are forbidden

elastic+helm-charts+pull-request+integration-elasticsearch/145/ES_SUITE=upgrade,KUBERNETES_VERSION=1.12

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm glad that upgrade test is in there and caught that!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems related to helm/charts#7680

@botelastic
Copy link

botelastic bot commented Jan 19, 2020

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. To track this PR (even if closed), please open a corresponding issue if one does not already exist.

4 similar comments
@botelastic
Copy link

botelastic bot commented Jan 19, 2020

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. To track this PR (even if closed), please open a corresponding issue if one does not already exist.

@botelastic
Copy link

botelastic bot commented Jan 19, 2020

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. To track this PR (even if closed), please open a corresponding issue if one does not already exist.

@botelastic
Copy link

botelastic bot commented Jan 19, 2020

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. To track this PR (even if closed), please open a corresponding issue if one does not already exist.

@botelastic
Copy link

botelastic bot commented Jan 19, 2020

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. To track this PR (even if closed), please open a corresponding issue if one does not already exist.

@jmlrt
Copy link
Member Author

jmlrt commented Jan 20, 2020

👍

@botelastic botelastic bot removed the triage/stale label Jan 20, 2020
@jmlrt jmlrt removed their assignment Feb 28, 2020
@botelastic
Copy link

botelastic bot commented May 28, 2020

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. To track this PR (even if closed), please open a corresponding issue if one does not already exist.

@jmlrt
Copy link
Member Author

jmlrt commented May 28, 2020

still valid

@botelastic botelastic bot removed the triage/stale label May 28, 2020
@jmlrt jmlrt added elasticsearch meta and removed enhancement New feature or request labels Jul 13, 2020
@botelastic
Copy link

botelastic bot commented Oct 11, 2020

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. To track this PR (even if closed), please open a corresponding issue if one does not already exist.

@jmlrt
Copy link
Member Author

jmlrt commented Oct 12, 2020

still valid

@botelastic botelastic bot removed the triage/stale label Oct 12, 2020
@jmlrt jmlrt marked this pull request as draft October 30, 2020 17:19
@botelastic
Copy link

botelastic bot commented Jan 28, 2021

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. To track this PR (even if closed), please open a corresponding issue if one does not already exist.

@jmlrt jmlrt closed this Feb 24, 2021
@jmlrt jmlrt deleted the cleanup-pv-after-tests branch February 24, 2021 10:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Crazybus Crazybus Crazybus left review comments

Assignees
No one assigned
Labels
elasticsearch meta triage/stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jmlrt @Crazybus